Skip to content

fix BaseModel.model_copy() seems to lose intersection data #3113#3547

Draft
asukaminato0721 wants to merge 2 commits into
facebook:mainfrom
asukaminato0721:3113
Draft

fix BaseModel.model_copy() seems to lose intersection data #3113#3547
asukaminato0721 wants to merge 2 commits into
facebook:mainfrom
asukaminato0721:3113

Conversation

@asukaminato0721
Copy link
Copy Markdown
Contributor

Summary

Fixes #3113

Test Plan

@github-actions

This comment has been minimized.

@asukaminato0721 asukaminato0721 marked this pull request as ready for review May 22, 2026 09:07
Copilot AI review requested due to automatic review settings May 22, 2026 09:07
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@github-actions

This comment has been minimized.

@asukaminato0721 asukaminato0721 marked this pull request as draft May 22, 2026 09:13
@github-actions
Copy link
Copy Markdown

Diff from mypy_primer, showing the effect of this PR on open source code:

tornado (https://github.com/tornadoweb/tornado)
+ ERROR tornado/simple_httpclient.py:651:29-49: Argument `HTTPRequest` is not assignable to parameter `self` with type `Self@HTTPRequest` in function `tornado.httpclient.HTTPRequest.headers` [bad-argument-type]

anyio (https://github.com/agronholm/anyio)
+ ERROR src/anyio/_core/_contextmanagers.py:46:25-39: Argument `ContextManagerMixin` is not assignable to parameter `self` with type `Self@object` in function `object.__class__` [bad-argument-type]
+ ERROR src/anyio/_core/_contextmanagers.py:65:20-34: Argument `ContextManagerMixin` is not assignable to parameter `self` with type `Self@object` in function `object.__class__` [bad-argument-type]
+ ERROR src/anyio/_core/_contextmanagers.py:85:25-39: Argument `ContextManagerMixin` is not assignable to parameter `self` with type `Self@object` in function `object.__class__` [bad-argument-type]
+ ERROR src/anyio/_core/_contextmanagers.py:132:25-39: Argument `AsyncContextManagerMixin` is not assignable to parameter `self` with type `Self@object` in function `object.__class__` [bad-argument-type]
+ ERROR src/anyio/_core/_contextmanagers.py:158:20-34: Argument `AsyncContextManagerMixin` is not assignable to parameter `self` with type `Self@object` in function `object.__class__` [bad-argument-type]
+ ERROR src/anyio/_core/_contextmanagers.py:177:25-39: Argument `AsyncContextManagerMixin` is not assignable to parameter `self` with type `Self@object` in function `object.__class__` [bad-argument-type]

dulwich (https://github.com/dulwich/dulwich)
+ ERROR dulwich/refs.py:1704:25-36: Argument `Tag` is not assignable to parameter `self` with type `Self@Tag` in function `dulwich.objects.Tag.object` [bad-argument-type]

bokeh (https://github.com/bokeh/bokeh)
+ ERROR src/bokeh/models/renderers/glyph_renderer.py:213:42-65: Argument `Image` is not assignable to parameter `self` with type `Self@Image` in function `bokeh.models.glyphs.Image.color_mapper` [bad-argument-type]

pandas (https://github.com/pandas-dev/pandas)
+ ERROR pandas/core/groupby/groupby.py:6017:26-34: Argument `Series` is not assignable to parameter `self` with type `Self@Series` in function `pandas.core.series.Series.name` [bad-argument-type]

artigraph (https://github.com/artigraph/artigraph)
- ERROR src/arti/types/pydantic.py:27:16-23: Returned type `Type | _NamedMixin` is not assignable to declared return type `Type` [bad-return]

@github-actions
Copy link
Copy Markdown

Primer Diff Classification

❌ 5 regression(s) | ✅ 1 improvement(s) | 6 project(s) total | +10, -1 errors

5 regression(s) across tornado, anyio, dulwich, bokeh, pandas. error kinds: bad-argument-type. caused by lookup_attr_from_attribute_base1_with_self_type(). 1 improvement(s) across artigraph.

Project Verdict Changes Error Kinds Root Cause
tornado ❌ Regression +1 bad-argument-type pyrefly/lib/alt/class/class_field.rs
anyio ❌ Regression +6 bad-argument-type lookup_attr_from_attribute_base1_with_self_type()
dulwich ❌ Regression +1 bad-argument-type lookup_attr_from_attribute_base1_with_self_type()
bokeh ❌ Regression +1 bad-argument-type pyrefly/lib/alt/class/class_field.rs
pandas ❌ Regression +1 bad-argument-type lookup_attr_from_attribute_base1_with_self_type()
artigraph ✅ Improvement -1 bad-return model_copy()
Detailed analysis

❌ Regression (5)

tornado (+1)

This is a false positive. self.request is typed as HTTPRequest (from the __init__ parameter at line 265), and accessing .headers on it should be valid. The error claims HTTPRequest is not assignable to parameter self with type Self@HTTPRequest in the headers property. When a property uses Self typing and is accessed on an instance of HTTPRequest, Self should be bound to HTTPRequest, making HTTPRequest trivially assignable to Self@HTTPRequest. Note that while there is an assert isinstance(self.request, _RequestProxy) at line 621 that could narrow the type, the error message explicitly shows the argument type as HTTPRequest, not _RequestProxy, confirming the issue is with Self-type substitution for HTTPRequest itself. The PR's changes to Self-type substitution appear to have broken Self-type resolution for properties, causing the type checker to reject a valid HTTPRequest instance as the self argument to its own property.
Attribution: The changes to subst_callable_return_self_type_mut in pyrefly/lib/alt/class/class_field.rs and the new InstanceKind::ClassWithSelfType variant in pyrefly/lib/alt/types/instance.rs changed how Self types are substituted in callable parameters vs return types. This appears to have introduced a regression where the self parameter of a property (like HTTPRequest.headers) is not properly resolved when accessed on a concrete instance, causing the spurious bad-argument-type error.

anyio (+6)

These are false positives. The code accesses self.__class__ inside methods where self has been narrowed via assert isinstance(self, ContextManagerMixin) (or AsyncContextManagerMixin). The __class__ property is defined on object and is accessible on every Python object. The error message says Argument ContextManagerMixin is not assignable to parameter self with type Self@object in function object.__class__ — this indicates pyrefly is incorrectly failing to match the intersection type against Self@object when resolving __class__. Neither mypy nor pyright flag any of these 6 locations. The PR's changes to intersection type self-type propagation in lookup_attr_from_attribute_base1_with_self_type() and subst_callable_return_self_type_mut() introduced a regression where the self parameter type for inherited object methods doesn't properly accept the intersection type.
Attribution: The PR changes how intersection types handle attribute lookups with self types. Specifically, in pyrefly/lib/alt/attr.rs, the AttributeBase1::Intersect variant now carries an Option<Type> for the self type, and lookup_attr_from_attribute_base1_with_self_type() is a new method that substitutes this self type when looking up attributes. In pyrefly/lib/alt/class/class_field.rs, the new subst_callable_return_self_type_mut() function substitutes the self type into callable return types but uses a fallback_self_type for parameters. The bug appears to be that when looking up object.__class__ (which has signature (self: Self@object) -> type[Self@object]), the intersection self type (e.g., ContextManagerMixin & _SupportsCtxMgr[...]) is being substituted into the parameter's Self@object, making it expect Self@object but receiving the concrete intersection type ContextManagerMixin. The subst_callable_return_self_type_mut() function in class_field.rs uses fallback_self_type for params (which is instance.to_type(heap), the plain class type) instead of the intersection type, causing a mismatch.

dulwich (+1)

The error occurs at line 1704 where head.object is accessed after isinstance(head, Tag). The variable head has static type ObjectID (a NewType of bytes) from refs[Ref(head_ref)]. After the isinstance check, the type is narrowed to the intersection ObjectID & Tag. Since Tag.object is typed with Self, the self parameter expects Self@Tag, but the intersection type ObjectID & Tag is not straightforwardly assignable to Self@Tag. Both pyrefly and pyright flag this error, which indicates it's a genuine type system concern rather than a false positive. The root cause is that the code assumes refs[Ref(head_ref)] can return a Tag object at runtime, but the declared return type is ObjectID (bytes), creating a type mismatch that surfaces when the isinstance narrowing creates an intersection type. This is a real type annotation issue in the source code, not a type checker bug.
Attribution: The change to lookup_attr_from_attribute_base1_with_self_type() in pyrefly/lib/alt/attr.rs and the new subst_callable_return_self_type_mut() in pyrefly/lib/alt/class/class_field.rs changed how Self types are substituted when accessing attributes through intersection types. The new code passes the intersection type as self_type but then uses fallback_self_type for parameters. This appears to cause the self parameter check to fail when accessing Tag.object through an ObjectID & Tag intersection.

bokeh (+1)

The error claims Image is not assignable to Self@Image when calling Image.color_mapper on line 213. After isinstance(self.glyph, (Image, ImageStack)), the type of self.glyph is narrowed to Image | ImageStack. When accessing .color_mapper on this union, the type checker must resolve the method for each union member. For the Image branch, Self@Image should be satisfied by Image, and for ImageStack, Self@ImageStack should be satisfied by ImageStack. An Image instance is always a valid Self for Image's own methods — this is tautological. The error appears to be a false positive, likely caused by the PR's changes to self-type handling incorrectly constraining the Self parameter when resolving attribute access on a union type narrowed by isinstance with a tuple argument. This is a type checker regression, not a real type error in the source code.
Attribution: The changes to pyrefly/lib/alt/class/class_field.rs (specifically subst_callable_return_self_type_mut and instantiate_for) and pyrefly/lib/alt/attr.rs (lookup_attr_from_attribute_base1_with_self_type) changed how Self types are substituted in intersection contexts. The new InstanceKind::ClassWithSelfType variant in pyrefly/lib/alt/types/instance.rs appears to be incorrectly propagating a self-type constraint when accessing color_mapper on a narrowed Image | ImageStack union, causing pyrefly to reject Image as not assignable to Self@Image.

pandas (+1)

This is a false positive regression. The PR introduces changes to how Self-returning methods handle intersection types, which has a side effect on property access. At line 6017, res has type NDFrameT (a TypeVar bound to NDFrame). Inside the isinstance(res, Series) check, the type is narrowed to Series & NDFrameT. When accessing res.name (a property on Series), the type checker needs to pass the intersection type as the self argument to the property getter. The name property on Series has self typed as Self@Series. The intersection type Series & NDFrameT should be assignable to Self@Series since it IS a Series (the intersection includes Series), but the new code path incorrectly rejects this assignment. This is a false positive - the code is valid Python and the property access should succeed.
Attribution: The change to lookup_attr_from_attribute_base1_with_self_type() in pyrefly/lib/alt/attr.rs and the new subst_callable_return_self_type_mut() in pyrefly/lib/alt/class/class_field.rs changed how Self types are resolved on intersection types. When res is narrowed to Series & NDFrameT inside the isinstance(res, Series) branch, the new code passes the intersection type as self_type when looking up name. The property getter's self: Self@Series parameter then fails to accept the intersection type, producing a spurious bad-argument-type error.

✅ Improvement (1)

artigraph (-1)

This is a clear improvement. The old error was a false positive caused by pyrefly incorrectly resolving Self-returning methods on intersection types. On line 24, subtype has type Type (the return type of to_artigraph). After the isinstance(subtype, _NamedMixin) check on line 25, subtype is narrowed to Type & _NamedMixin (an intersection type). When model_copy() is called on line 26 (which returns Self), pyrefly should resolve the Self return type as Type & _NamedMixin — the same intersection type. Since an intersection Type & _NamedMixin is a subtype of both Type and _NamedMixin, it is assignable to the declared return type Type. However, pyrefly was previously incorrectly producing Type | _NamedMixin (a union) instead of the correct intersection. In a union, the _NamedMixin branch is not necessarily a subtype of Type, so pyrefly raised a spurious bad-return error. The PR fixes the Self type substitution for intersection types, eliminating this false positive.
Attribution: The fix is in pyrefly/lib/alt/class/class_field.rs where subst_callable_return_self_type_mut was added. This new method handles the case where an instance has a self_return_type_override (the intersection type). For callable return types, it substitutes the intersection type as Self, while for parameters it uses the fallback type. The Instance::of_class_with_self_type variant in pyrefly/lib/alt/types/instance.rs carries the intersection type through attribute lookups. The changes in pyrefly/lib/alt/attr.rs (lookup_attr_from_attribute_base1_with_self_type) ensure that when looking up attributes on intersection types, the self type is properly propagated. The test case test_self_returning_method_on_typevar_intersection directly demonstrates this fix: value.[model_copy()](https://github.com/facebook/pyrefly/blob/main/pyrefly/lib/alt/class/class_field.rs) on ChildModel & T now correctly returns ChildModel & T instead of producing a union.

Suggested fixes

Summary: The PR's subst_callable_return_self_type_mut() uses fallback_self_type for parameter Self substitution while the actual self argument is the intersection type, causing bad-argument-type errors on properties and methods accessed through intersection types.

1. In subst_callable_return_self_type_mut() in pyrefly/lib/alt/class/class_field.rs, change the parameter substitution to also use self_type (the intersection type) instead of fallback_self_type. Currently the function substitutes self_type into return types but fallback_self_type into params. Since the actual self argument IS the intersection type, the parameter's Self must also accept the intersection type. Change callable.params.visit_mut(&mut |ty| ty.subst_self_type_mut(fallback_self_type)) to callable.params.visit_mut(&mut |ty| ty.subst_self_type_mut(self_type)) — effectively making both return and parameter Self substitution use the same self_type. This means the fallback_self_type parameter can be removed entirely, and all callers simplified. The key insight is that Self in parameters and Self in returns must be substituted consistently; the original split was the source of all 10 regressions.

Files: pyrefly/lib/alt/class/class_field.rs
Confidence: high
Affected projects: tornado, anyio, bokeh, pandas, dulwich
Fixes: bad-argument-type
All 10 regression errors (tornado:1, anyio:6, bokeh:1, pandas:1, dulwich:1) are bad-argument-type errors where an intersection type or concrete class type fails to match Self@ClassName in a parameter position. The root cause is that subst_callable_return_self_type_mut substitutes the intersection type into returns but the plain class type into params. When the caller passes the intersection-typed self, it doesn't match the plain-class-typed parameter. Using the intersection type for both positions fixes this: Self@Image becomes Image&X in both param and return, and the Image&X argument matches Image&X parameter. The artigraph improvement (return type fix) is preserved since return substitution is unchanged. The 9 pyrefly-only errors across tornado, anyio, bokeh, and pandas would be eliminated. The 1 dulwich error (co-reported by pyright) would also be fixed but that's acceptable collateral.


Was this helpful? React with 👍 or 👎

Classification by primer-classifier (6 LLM)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BaseModel.model_copy() seems to lose intersection data

2 participants